format fromBlock param on resubscription #3596
Conversation
There was a problem hiding this comment.
Thanks for catching this @epiccoolguy!
What do you think about moving the fix into the re-subscription logic that comes slightly earlier than where you've currently placed it?
It looks like the bug is introduced here, when lastBlock gets assigned an output formatted blockNumber
...so this only affects resubscriptions (as you've noted).
Re: tests - this looks ok to me without an additional test. There is a logs backfilling test here which hits these lines but it's only running vs. ganache (which must be less strict about the RPC spec than besu).
…fering with its presence check" This reverts commit a629577.
|
Hey @cgewecke! I initially added the formatting up there, but was worried about isFinite taking a number type value, but after reading the docs, I found out it'll convert it to number if required: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/isFinite I'll revert it back to formatting on assignment. If it's not necessary to test the output being formatted properly, I'd leave the PR as is. |
|
Thanks @epiccoolguy! |
Description
Added use of
inputBlockNumberFormatterto ensurefromBlockis formatted properly when resubscribing. Currently when resubscribing, the plain incremented number field is put into the RPC call.The JSON-RPC docs mention that values like fromBlock should be hex encoded. I ran into an issue where Besu will return "Invalid Params" when web3 passes in the plain number value when reconnecting on a flaky WS connection: hyperledger/besu#1088.
I'm not familiar with the web3 codebase, so it would be great if someone could point me in the right direction for adding/updating tests.
Type of change
Checklist:
npm run dtslintwith success and extended the tests and types if necessary.npm run test:unitwith success.npm run test:covand my test cases cover all the lines and branches of the added code.npm run build-alland tested the resulting files fromdistfolder in a browser.CHANGELOG.mdfile in the root folder.